Skip to content

Various fixes to fix the "we don't choose direct routes" problem. - #2307

Merged
cdecker merged 8 commits into
ElementsProject:masterfrom
rustyrussell:guilt/route-increase-cltv-penalty
Feb 6, 2019
Merged

Various fixes to fix the "we don't choose direct routes" problem.#2307
cdecker merged 8 commits into
ElementsProject:masterfrom
rustyrussell:guilt/route-increase-cltv-penalty

Conversation

@rustyrussell

@rustyrussell rustyrussell commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

This was something of a saga:

  1. The test looped 8 times, but because our RNG is deliberately fixed, that didn't change our route.
  2. pay tries routehints first, so sometimes we went indirect because we were following the routehint
    (which did change around the loop).
  3. The test case only covered the simple case; a more sophisticated test reveals it's a route evaluation error.
  4. We completely mishandled riskfactor in multiple(!) ways, and even when fixed it's in the noise.

Fixes: #2119
Closes: #2168
Closes: #2144

ZmnSCPxj and others added 3 commits February 1, 2019 01:01
@ZmnSCPxj @rustyrussell
test_pay.py: Add test that we prefer direct route.
e8d571b
@rustyrussell
gossipd: increase randomness in route selection.
b49fa5f 
We have a seed, which is for (future!) unit testing consistency.  This
makes it change every time, so our pay_direct_test is more useful.

I tried restarting the noed around the loop, but it tended to fail
rebinding to the same port for some reason?

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
plugins/pay: try without routehints first.
f10dc44 
This is the direct cause of the failure of the original
test_pay_direct test and it makes sense: invoice routehints may not be
necessary, so try without them *first* rather than last.

We didn't mention the use of routehints in CHANGELOG at all yet, so
do that now.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell added Type::Bug An error, flaw, or fault that produces an incorrect or unexpected result routing labels Feb 1, 2019
@rustyrussell rustyrussell added this to the v0.6.4 milestone Feb 1, 2019
@rustyrussell
rustyrussell force-pushed the guilt/route-increase-cltv-penalty branch from 11de198 to 09de978 Compare February 1, 2019 03:39
@rustyrussell
pytest: make test_pay_direct more effective.
b8389d2 
The test sometimes passes: our routing logic always chooses between
the shorter of two equal-cost routes (because we compare best with <
not <=).

By adding another hop, we add more noise, and by making the alternate
route fee 0 we provide the worst case.

But to be fair, we make the amount of the payment ~50c (15,000,000
msat), and increase our cltv-delay to 14 and fee-base 1000 to match
mainnet.  The final patch shows the effect of this choice.

Otherwise our risk penalty is completely in the noise on
mainnet which has the vast majority of fees set at 1000msat + 1ppm.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
gossipd: fix riskfactor passing.
a5434e3 
We used a u16, and a 1000 multiplier, which meant we wrapped at
riskfactor 66.  We also never undid the multiplier, so we ended up
applying 1000x the riskfactor they specified.

This changes us to pass the riskfactor with a 1M multiplier.  The next
patch changes the definition of riskfactor to be more useful.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
gossipd: take into account risk in final route comparison.
7b0973b 
We were only comparing by total msatoshis.

Note, this *still* isn't sufficient to fix our indirect problem, as
our risk values are all 1 (the minimum):

	lightning_gossipd(25480): 2 hop solution: 1501990 + 2
	lightning_gossipd(25480): 3 hop solution: 1501971 + 3
	...
	lightning_gossipd(25480): => chose 3 hop solution

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
getroute: change definition (and pay default) for riskfactor.
8e210e9 
Up until now, riskfactor was useless due to implementation bugs, and
also the default setting is wrong (too low to have an effect on
reasonable payment scenarios).

Let's simplify the definition (by assuming that P(failure) of a node
is 1), to make it a simple percentage.  I examined the current network
fees to see what would work, and under this definition, a default of
10 seems reasonable (equivalent to 1000 under the old definition).

It is *this* change which finally fixes our test case!  The riskfactor
is now 40msat (1500000 * 14 * 10 / 5259600 = 39.9), comparable with
worst-case fuzz is 50msat (1001 * 0.05 = 50).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
rustyrussell force-pushed the guilt/route-increase-cltv-penalty branch from 09de978 to 8e210e9 Compare February 1, 2019 06:15
ghost
ghost reviewed Feb 1, 2019
View reviewed changes
Comment thread doc/lightning-getroute.7.txt
during the process. The 'riskfactor' floating-point field controls
this tradeoff; it is the annual cost of your funds being stuck (as a
percentage), multiplied by the percentage chance of each node failing.
percentage).

@ghost ghost Feb 1, 2019 •
edited by ghost
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

percentage of what? channel capacity? Sorry, read the full context now, percentage of the payment amount

@rustyrussell
pytest: don't time out on test_pay_direct !DEVELOPER
bc1c0af 
Travis timed out.

Waiting for three fundchannel commands depends on the bitcoind polling
interval (30 seconds), and then waiting for gossip propagation
requires two propagation intervals (120 seconds).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker

cdecker commented Feb 4, 2019

Copy link
Copy Markdown
Member

ACK bc1c0af

Though I seem to have gotten lost in the maxfeepercent vs fuzz factor vs riskfactor logic: a 5% fuzz allows us to use routes that are up to 5% worse than what exactly?

niftynei
niftynei approved these changes Feb 4, 2019
View reviewed changes
Comment thread plugins/pay.c
pc->excludes = tal_arr(cmd, const char *, 0);
pc->ps = add_pay_status(pc, b11str);
/* We try first without using routehint */
pc->current_routehint = NULL;

@niftynei niftynei Feb 4, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨

Comment thread doc/lightning-getroute.7.txt
fail, and it would cost you 20% per annum if that happened,
'riskfactor' would be 20.
For example, if you thought the inconvenience of having funds stuck was
worth 20% per annum interest, 'riskfactor' would be 20.

@niftynei niftynei Feb 4, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restating this to confirm I'm understanding: riskfactor is a way of dynamically valuing shorter routes, by accounting for the potential loss of liquidity in the case that a payment gets stuck along the route. i.e. the more hops in a route (and payment value) the higher the riskfactor. In some sense, this makes higher fee yet shorter routes more attractive as the possibility for getting stuck is lowered due to less chances of a link failure en route.

@cdecker
cdecker merged commit fbf2168 into ElementsProject:master Feb 6, 2019
@ghost ghost mentioned this pull request Feb 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niftynei niftynei niftynei approved these changes
@cdecker cdecker Awaiting requested review from cdecker
@ZmnSCPxj ZmnSCPxj Awaiting requested review from ZmnSCPxj
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

routing Type::Bug An error, flaw, or fault that produces an incorrect or unexpected result

Projects

None yet

Milestone

v0.6.4

Development

Successfully merging this pull request may close these issues.

Direct channel should be preferred

4 participants

@rustyrussell @cdecker @niftynei @ZmnSCPxj